Skip to content

Philip security fixes#606

Closed
Philip-Carneiro-KX wants to merge 7 commits intodevfrom
philip-security-fixes
Closed

Philip security fixes#606
Philip-Carneiro-KX wants to merge 7 commits intodevfrom
philip-security-fixes

Conversation

@Philip-Carneiro-KX
Copy link
Contributor

Changes introduced by this PR

  • Fix vulnerable to super-linear runtime due to backtracking, cannot lead to denial of service.

@Philip-Carneiro-KX Philip-Carneiro-KX self-assigned this Jun 25, 2025
@ecmel
Copy link
Collaborator

ecmel commented Jun 25, 2025

@Philip-Carneiro-KX This is not a real security issue.

@Philip-Carneiro-KX
Copy link
Contributor Author

Philip-Carneiro-KX commented Jun 25, 2025

@Philip-Carneiro-KX This is not a real security issue.

Security Vulnerability Analysis: Regex Denial of Service (ReDoS)

Yes, this is a legitimate security vulnerability known as ReDoS (Regex Denial of Service). Here's why:

1. Problematic Regex Pattern

\/[\s\S]*?\\
  • [\s\S]*? matches any character (including newlines) in a non-greedy/lazy way.
  • The pattern tries to find the shortest possible sequence between / and \.

2. Backtracking Vulnerability

When the ending \ is missing in the input string:

  • The regex engine tries every possible position to find \.
  • For an input like "/a" + "x".repeat(1_000_000):
    • The engine scans the entire string (1M characters) to confirm no \ exists.
    • Worst-case time complexity: O(n) (linear).

3. Why This is a Security Risk

  • Super-linear runtime: While this specific regex is "only" O(n), it becomes dangerous when:
    • Inputs are extremely large (e.g., user-generated content).
    • Used in sensitive contexts (e.g., server-side processing).
  • Denial of Service: An attacker could submit a maliciously long string without the ending \, causing:
    • CPU spikes.
    • Server unresponsiveness.
    • Resource exhaustion.

4. Real-World Impact

// Example attack payload
const maliciousInput = `/${'a'.repeat(100_000_000)}`; // 100M characters
  • Processing this with .replace(/\/[\s\S]*?\\/g, "") would freeze the Node.js event loop for seconds/minutes.

5. How to Fix It

Option 1: Use a Parser Instead
For complex syntax (like block comments), avoid regex. Use a proper parser/tokenizer.

Option 2: Safeguard the Regex

  • Add timeouts: Use libraries like safe-regex or re2.
  • Limit input size: Restrict max length before regex processing.
  • Optimize the pattern:
\/[\s\S]*?\\/

Still risky. Prefer parser-based solutions.

Key Takeaway

While your regex isn’t catastrophically exponential (like nested quantifiers), its linear worst-case behavior on large inputs makes it vulnerable to DoS attacks. Always validate input size and prefer non-regex solutions for complex parsing.

@kx-sonarqube
Copy link

kx-sonarqube bot commented Jun 25, 2025

@ecmel
Copy link
Collaborator

ecmel commented Jun 25, 2025

@Philip-Carneiro-KX I meant in the real world use cases, before adding 100+ million chars in a file, the Monaca editor in vscode itself and vscode will probably crash. This is no a server side software, we are dealing with a language server extension here, it is running on vscode extension host sandbox, and ultimately on nodejs so we should use regexes when dealing with source code. We can modify the regex instead of writing and maintaining a parser just for this.

@Philip-Carneiro-KX Philip-Carneiro-KX deleted the philip-security-fixes branch June 25, 2025 15:45
@ecmel ecmel mentioned this pull request Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments